Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a frame_id field to LocationFix #108

Merged
merged 3 commits into from
May 11, 2023

Conversation

defunctzombie
Copy link
Contributor

@defunctzombie defunctzombie commented May 9, 2023

The frame_id field for the LocationFix message specifies the frame for the origin of the sensor so the lat/lon values can be tied to a location on the robot.

Fixes: #100


Nothing in Studio uses this information yet but we can introduce this for folks that want to start recording it.

The frame_id field for the LocationFix message specifies the frame for the origin of the sensor so the lat/lon values can be tied to a location on the robot.
@defunctzombie defunctzombie requested review from jtbandes and snosenzo and removed request for jtbandes May 9, 2023 03:51
Copy link
Contributor

@snosenzo snosenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this is the right way to add this? I think this might need a headerstamp to address a frameId at a specific header time as well, no?

Is there a reason we're doing it this way and not adding a new TransformWithLocationFix message?
Just trying to think about what usage would look like

@defunctzombie
Copy link
Contributor Author

I think this might need a headerstamp to address a frameId at a specific header time as well, no?

Yea - we need to add a timestamp field like we have with other messages as well.

Is there a reason we're doing it this way and not adding a new TransformWithLocationFix message?

What would this message be?

To me the reason to go with this approach is that it uses the same pattern that other sensor data uses which is that there is a frame_id for the sensor and some message data tied to that frame.

@snosenzo
Copy link
Contributor

snosenzo commented May 9, 2023

To me the reason to go with this approach is that it uses the same pattern that other sensor data uses which is that there is a frame_id for the sensor and some message data tied to that frame.

Ah yeah, that makes sense. I guess defining a new kind of transform message would make it harder to access this, so nevermind on that.

Yeah, so all this needs really is the timestamp.

@defunctzombie
Copy link
Contributor Author

I've added a timestamp.

"Frame for the sensor. Latitude and longitude readings are at the origin of the frame.",
},
{
name: "timestamp",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: every other schema has timestamp listed before frame_id

// Latitude in degrees
double latitude = 1;
double latitude = 3;
Copy link
Contributor

@amacneil amacneil May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that it doesn't affect us directly due to mcap storing schemas in the file, but it seems against the spirit of protobuf to change these field identifiers right? It would break backwards compatibility for anyone trying to use new schemas to read old data?

Not sure if we have already made breaking changes like this before?

Copy link
Contributor

@amacneil amacneil May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted to solve this while still allowing the fields to appear in desired order (timestamp at the top), we might need to introduce some explicit protobuf field ID into schemas.ts to manually assign these. I think it's only an issue for protobuf.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtbandes Have we done anything like this yet?

@amacneil I think we roughly treat these schemas as a snapshot set of examples rather than "use these exactly for all possible use-cases you have". Maybe we should call out this nuance in protobuf more explicitly but I've never known this repo to have a goal of keeping things backwards compatible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we have, search for protobufFieldNumber

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we roughly treat these schemas as a snapshot set of examples rather than "use these exactly for all possible use-cases you have". Maybe we should call out this nuance in protobuf more explicitly but I've never known this repo to have a goal of keeping things backwards compatible.

Yeah agreed I don't think we explicitly make this promise. But it does seem a bit weird from a user's perspective for us to unnecessarily ship backwards-incompatible changes to the schema. So maybe we should make that promise going forward unless there is a particular change where that would be impossible. In this case looks like we just need to explicitly specify the numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip on protobufFieldNumber I've added that to LocationFix to keep the existing field numbering unchanged.

Copy link
Contributor

@snosenzo snosenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably make a ticket for testing that we don't break protobuf field numbers when updating the schema if there isn't one already

@defunctzombie defunctzombie merged commit f8d19d0 into main May 11, 2023
@defunctzombie defunctzombie deleted the roman/fg-2726-add-frame_id-to-locationfix branch May 11, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add frame_id to LocationFix
4 participants